-
-
Notifications
You must be signed in to change notification settings - Fork 18k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PERF: avoid going through .values for reindexing both index/columns for ArrayManager #44798
PERF: avoid going through .values for reindexing both index/columns for ArrayManager #44798
Conversation
I've been looking at this code path as, for reasons related to the CI failures, it appears fragile. e.g. in TestDataFrameSelectReindex.test_reindex_datetimelike_to_object setting |
It also seems that our deprecation is not working if you only reindex the rows (and not the rows/columns together):
The second should also work and trigger the deprecation warning (the tests here are running into this because I am now basically taking the reindex path in separate steps for AM). Which is probably indeed the same as #42921 |
IIRC this code path goes through maybe_promote, which doesn't quite match the DTA behavior. There was some work a few months ago that brought them closer into alignment, but off the top of my head I don't remember how far that got. |
seems reasonable can you merge master |
if ( | ||
row_indexer is not None | ||
and col_indexer is not None | ||
and not isinstance(self._mgr, ArrayManager) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think a more general condition would be self._can_fast_transpose
. The thing we care about is self.values being cheap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorisvandenbossche can you address comments and rebase
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
xref #39146
This fastpath calls
self.values
and thus is only a fastpath for BlockManager (we only get here with all columns having the same dtype).